Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Clone bound on Application::Message #155

Merged
merged 5 commits into from
Jan 13, 2020

Conversation

ejmahler
Copy link
Contributor

@ejmahler ejmahler commented Jan 13, 2020

I'm currently writing an application where I want to create a message that should not be cloned.

It's a Box<RenderState> where RenderState contains a bunch of wgpu handles to GPU resources. I could wrap it in Arc<Mutex<>> and pass it through a message that way, but being able to pass it as a box achieves the same thing more clearly.

I can't do this in master, because the Message associated type has a bound on Clone. But it turns out that nothing actually takes advantage of this bound, and IMO it's logical that the system should only ever be moving messages, not cloning them.

This change will put a greater restriction on how Message is used, but it's a restriction that the system appears to be currently handling fine.

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

Although we can remove it in Application, many native widgets (like Button and TextInput) require this bound. There are also some features that will need this bound in Application at some point, like a time travelling debugger. It's a safe default.

Why do you want to send GPU resources through a Message? They are meant to contain simple data. If it's the result of some computation, using Arc is the proper way to do it.

However, I'd advise you against using a Mutex. Chances are what you are trying to do can be achieved by using a better data model.

@hecrj hecrj added the question Further information is requested label Jan 13, 2020
@ejmahler
Copy link
Contributor Author

I have a Command that triggers a compute shader, and a message that returns the results.

Commands have a bound of static on their futures, which means the futures need to take ownership of their parameters -- so the future needs ownership over the compute resources. That means either box or arc.

Arc is problematic though, because my command needs mutable access to its GPU resources. So a mutex would be required if I used arc. So the simplest thing to do is box. When starting the computation, I move the box into the future, and when the computation, it moves the box back via the message. The nice thing about this design is that it also prevents multiple commands from accessing the compute resources at once, at compile time. Because you have to move the box containing the resources into the command, there's no possibility of reusing it to issue another command later which will wait on a mutex.

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

How does a Box let you move data from your Application into a future? What is left behind while the future is running? Isn't what you need an Option and Option::take?

Why do you need to share a RenderState between invocations of the Command? Could the Command create the state every time from scratch based on some other input? What does your application do? A bit more details would help me understand your use case and give you further guidance!

From what I gathered, I feel like you are trying to own state that shouldn't be yours. In other words, I think you should design your renderer as a separate runtime and then have a communication layer between it and your GUI.

One way to achieve this is by making your renderer live in a Subscription and communicate with your app using a channel (mpsc should work). The renderer will own its state and will be able to mutate it and the application won't see it (no need to move it or borrow it). It will also be able to notify the application of progress by returning messages (a Subscription is really just a Stream).

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

In any case, this PR doesn't really remove the bounds found in the widgets themselves, just the Application. You will most likely hit the same issue at some point if you plan to use a Button, TextInput, Element::map, etc. There is also the fact that there are plans to use this bound for future features.

If the Subscription idea sounds too complicated, you may be able to leverage Arc::try_unwrap or Arc::get_mut to obtain ownership or a mutable reference, respectively. No need for a Mutex.

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

I gave it some more thought, and I think your changes are worth merging.

Although I would tackle your particular use case differently, I think non-cloneable messages can be useful for other scenarios.

For instance, you can achieve what you want and still be able to use widgets that need a Clone requirement by modelling your Message like this:

#[derive(Debug)]
enum Message {
    Interaction(Interaction),
    RenderingFinished(RenderState),
}


#[derive(Debug, Clone, Copy)]
enum Interaction {
    ButtonPressed,
    // ... Other interactions you may be interested in!
}

Notice that Message does not implement Clone, so you should be able to use a RenderingFinished without an Arc.

Additionally, I have removed the Clone requirement for Element::map, so you should be able to turn any Element<Interaction> into an Element<Message> easily:

impl Application for Example {
    // ...

    fn view(&mut self) -> Element<Message> {
        let content: Element<Interaction> =
            Button::new(&mut self.increment_button, Text::new("Press me!"))
                .on_press(Interaction::ButtonPressed)
                .into();

        content.map(Message::Interaction)
    }
}

Thank you for helping me notice this approach! I didn't think it was possible.

About the time travelling debugger, I imagine we could implement it with a trait extension. We can ask for Message: Clone or similar there!

Finally, I have removed Clone requirements from Sandbox and some web widgets. Let me know what you think, and we can merge this!

@hecrj hecrj added feature New feature or request improvement An internal improvement and removed question Further information is requested labels Jan 13, 2020
@ejmahler
Copy link
Contributor Author

How does a Box let you move data from your Application into a future? What is left behind while the future is running? Isn't what you need an Option and Option::take?

Why do you need to share a RenderState between invocations of the Command? Could the Command create the state every time from scratch based on some other input? What does your application do? A bit more details would help me understand your use case and give you further guidance!

The ultimate goal is to show a fractal render in-progress. I have a fractal that takes 100k iterations to complete. I'd like to make a system where you can request that it run N iterations and then return incremental progress back to the UI.

So the design is, there's a Command to run N iterations, and when it's done it sends a message back that it's done. The message handler will spawn 2 commands when this happens. The first command converts the fractal data into an Image handle for rendering, and the second command is to continue computations. Finally, when the fractal is completely done, it either just drops the GPU state right there, or stores it on the GUI object. I want to try the former first.

So I don't want to recreate the render state every time because I imagine the steps will be quite short (N might be 1 to 10).

From what I gathered, I feel like you are trying to own state that shouldn't be yours. In other words, I think you should design your renderer as a separate runtime and then have a communication layer between it and your GUI.

One way to achieve this is by making your renderer live in a Subscription and communicate with your app using a channel (mpsc should work). The renderer will own its state and will be able to mutate it and the application won't see it (no need to move it or borrow it). It will also be able to notify the application of progress by returning messages (a Subscription is really just a Stream).

I had never heard of subscriptions before. I'm willing to try this out!

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

Thanks for the details! Sounds really cool. If you end up open-sourcing it, I'd love to try it out!

I think event subscriptions are the best way to do what you want. They are basically the Stream version of Command and can be used nicely to model asynchronous systems that need to persist and report progress like WebSocket connections, HTTP downloads, etc.

Event subscriptions were merged recently in #122. Feel free to ask any questions in the Zulip server.

impl<Message> Clone for Bus<Message> {
fn clone(&self) -> Self {
Self {
publish: Rc::clone(&self.publish),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you discovered the same thing I did, where #[derive(Clone)] seemsto generate a wonky implementation of clone here, which only kicks in if Message is Clone -- even though all the fields are trivially cloneable unconditionally. And the solution isto just manually implement an unconditional clone.

@ejmahler
Copy link
Contributor Author

ejmahler commented Jan 13, 2020

I'll be honest, your last example there is over my head. I'm not super clear on the difference between interactions, elements, messages, and Element<Interaction> and Element<message>

One concern is, how discoverable do you think it is to have to put element interactions inside an inner message? I can imagine the following sequence:

  1. The developer makes a message, which works for something trivial
  2. The developer tries to add a button press, which gives them a compile error about needing clone, so on autopilot, the developer adds a Clone derive on their whole message struct
  3. The developer never learns that they can remove the Clone derive.

I suppose it's fine if they do - something like this would probably be a pretty advanced usage. It it's worth thinking about.

@ejmahler
Copy link
Contributor Author

Ah, I see my confusion. I thought interaction was a built-in thing I hadn't seen before, and content.map basically tells the content widget to map its messages via the function you supply?

How many people will discover that you can do something like content.map, and how many people will understand what they're reading when they find it in someone else's code?

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

@ejmahler The Elm Architecture composes quite nicely. The whole idea is that your Message type does not need to be flat. In fact, nesting messages is an interesting technique that can be used to compose different smaller applications into a bigger one. The Element::map documentation shows an example of this.

The developer never learns that they can remove the Clone bound.

The Clone bound is still needed, but only by the message type you use directly in your widgets. If your usage is advanced enough that you need an un-cloneable Application::Message, you can simply nest the bound. I don't think this strategy is that uncommon.

@ejmahler
Copy link
Contributor Author

Yeah, I agree that the idea of nesting messages should feel pretty natural. I developed something for this a while ago and I reached for nesting messages almost immediately.

This looks good then.

@hecrj
Copy link
Member

hecrj commented Jan 13, 2020

How many people will discover that you can do something like content.map, and how many people will understand what they're reading when they find it in someone else's code?

Element::map is no different than Option::map or Iterator::map.

The tour and todos examples use it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request improvement An internal improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants